Skip to content

[improvement](be) Add CPU-aware S3 rate limiter#65420

Open
0AyanamiRei wants to merge 15 commits into
apache:masterfrom
0AyanamiRei:feature/cpu-aware-s3-limiter
Open

[improvement](be) Add CPU-aware S3 rate limiter#65420
0AyanamiRei wants to merge 15 commits into
apache:masterfrom
0AyanamiRei:feature/cpu-aware-s3-limiter

Conversation

@0AyanamiRei

@0AyanamiRei 0AyanamiRei commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary:

The existing S3 GET/PUT token bucket rate limiter uses fixed per-BE QPS settings. This is not suitable for heterogeneous deployments because a BE with many CPU cores and a BE with few CPU cores receive the same S3 request budget.

This PR adds CPU-aware S3 rate limiter settings for GET and PUT requests. When the new per-core settings are left as 0, Doris keeps the existing absolute limiter behavior. When a per-core setting is positive, Doris derives the effective per-BE S3 QPS from BE CPU cores and caps it with s3_token_per_second_max when configured.

The PR also centralizes effective limiter config calculation, reuses it for initialization and dynamic rebuild, and guards CPU-derived QPS calculation from overflow before applying the max cap.

Release note

Add BE configs to support CPU-aware S3 GET/PUT request rate limiting.

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

0AyanamiRei and others added 9 commits July 9, 2026 15:14
Add three new CPU-aware S3 limiter configs:
- s3_get_token_per_second_per_core: GET QPS per CPU core (0 to disable)
- s3_put_token_per_second_per_core: PUT QPS per CPU core (0 to disable)
- token_per_second_max: Hard cap on derived QPS (0 means no cap)

When per-core values are > 0, effective QPS = per_core_value * CpuInfo::num_cores()
When per-core values are = 0, existing absolute configs are used (preserving backward compatibility).

All new configs default to 0, maintaining existing behavior.

Affected files:
- be/src/common/config.h: Added DECLARE statements
- be/src/common/config.cpp: Added DEFINE statements with validators

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix naming inconsistency: all other S3 rate-limiter config options carry the
s3_ prefix. This public API change must be fixed now before downstream code
starts using these configs.

- DECLARE_mInt64(s3_token_per_second_max) in config.h
- DEFINE_mInt64(s3_token_per_second_max) in config.cpp
- DEFINE_Validator(s3_token_per_second_max) in config.cpp

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add S3RateLimiterConfig struct and get_effective_s3_rate_limiter_config()
to centralize the logic for computing effective rate limiter parameters
from both legacy configs and new per-core configs.

Changes:
- Add S3RateLimiterConfig struct in s3_util.h
- Add get_effective_s3_rate_limiter_config() declaration and implementation
- Add helper functions checked_positive_product() and apply_s3_token_per_second_max()
- Add required includes: <limits> and util/cpu_info.h

This is Task 2 of the S3 rate limiter refactoring.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n and dynamic reset

Replace raw config::s3_{get,put}_* accesses with get_effective_s3_rate_limiter_config()
in both S3ClientFactory constructor (initialization) and check_s3_rate_limiter_config_changed()
(dynamic reset). This ensures per-core QPS scaling and the s3_token_per_second_max cap are
applied consistently at startup and on every dynamic config check.

Also add an INFO log on successful limiter reset to make config changes observable in logs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cover four scenarios: absolute GET config, per-core GET without cap,
per-core GET with cap, and per-core PUT with cap. Add
S3RateLimiterConfigGuard RAII helper to save/restore all nine related
configs after each test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace manual save/restore of 3 config values with S3RateLimiterConfigGuard
that handles all 9 relevant S3 rate limiter configs. This prevents state
contamination between tests.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@0AyanamiRei 0AyanamiRei changed the title [feature](be) Add CPU-aware S3 rate limiter [improvement](be) Add CPU-aware S3 rate limiter Jul 9, 2026
### What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: CPU-aware S3 rate limiter calculation multiplied per-core QPS by BE CPU cores before applying s3_token_per_second_max. Extremely large mutable per-core settings could overflow int64 and trigger a CHECK even when a valid max cap was configured. This change computes the CPU-derived QPS with a wider intermediate value and applies the configured cap safely. It also adds unit coverage for the overflow-with-cap case and PUT limiter rebuild when the max cap changes.

### Release note

None

### Check List (For Author)

- Test: Unit Test

    - ./run-be-ut.sh --run --filter=S3UTILTest.effective_s3_rate_limiter_config_caps_overflowing_per_core_get_config

    - ./run-be-ut.sh --run --filter='S3UTILTest.*'

- Behavior changed: No

- Does this need documentation: No
@0AyanamiRei 0AyanamiRei force-pushed the feature/cpu-aware-s3-limiter branch from 5287ce9 to a4effbe Compare July 9, 2026 11:24
### What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: The S3 bytes limiter ignored the result of add(), unlike the request QPS limiter, and the per-core bandwidth configs defaulted to disabled. Check the bytes limiter result consistently and set GET/PUT per-core defaults to 100 MiB/s.

### Release note

Set default per-core S3 GET/PUT bandwidth limits to 100 MiB/s and consistently handle limiter failures.

### Check List (For Author)

- Test: No need to test (per user request; format checks only)
- Behavior changed: Yes (S3 bandwidth limiting defaults to 100 MiB/s per CPU core)
- Does this need documentation: No
@sollhui

sollhui commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one blocking issue in the S3 bandwidth limiter propagation.

Critical checkpoint conclusions:

  • Goal/test coverage: The PR adds CPU-aware S3 request and byte limiters and BE unit tests for config calculation and dynamic rebuild. It does not fully apply the new byte limiter to every object-storage path that already shares the S3 rate-limiter contract.
  • Scope/focus: The core PR scope is focused on S3 limiter config, S3 client enforcement, and tests.
  • Concurrency/lifecycle: Limiter reset keeps the existing holder locking and atomic update guards; I did not find a separate concurrency issue in the authoritative PR file set.
  • Config/compatibility: The new mutable BE configs are dynamically observed through the existing checker, but the shared Azure path is left incompatible with the new bandwidth limiter behavior.
  • Parallel paths: Native S3 charges bytes for PUT/UploadPart/GetObject; Azure still charges only request count through the same shared limiter gate.
  • Tests/CI: The added unit tests cover S3 effective config and rebuild behavior, but not the Azure parallel path. I could not run BE UTs in this runner because the checkout lacks thirdparty/installed and protoc. git diff --check was clean for the GitHub PR files; check-format.sh could not run because clang-format 16 is unavailable.

User focus: no additional user-provided review focus was supplied.

Comment thread be/src/io/fs/s3_obj_storage_client.cpp Outdated
if (sleep_duration < 0) {
return T(s3_error_factory());
}
if (bytes > 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The byte limiter only gets charged on the native S3 path. Azure still uses the same enable_s3_rate_limiter/S3ClientFactory::rate_limiter() helper, but its put_object, upload_part, and get_object calls never pass stream.size() or bytes_read through bytes_rate_limiter(). With the new positive defaults for s3_get_bytes_per_second_per_core and s3_put_bytes_per_second_per_core, enabling the rate limiter now throttles S3 bandwidth but silently leaves Azure bandwidth unlimited. Please either apply the same byte accounting to the Azure read/write helpers or make these new configs/API explicitly S3-native so Azure users do not expect them to apply.

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#65420

Problem Summary: The CPU-aware S3 bandwidth limiter was applied only to the native S3 client. Azure shared the request limiter but did not charge bytes for uploads or downloads, so enabling the limiter produced inconsistent behavior. Centralize request and byte limiter acquisition and pass payload sizes from Azure put, multipart upload, and get operations.

### Release note

Apply object-storage bandwidth limits consistently to native S3 and Azure Blob Storage.

### Check List (For Author)

- Test: Unit Test
    - `S3UTILTest.*`
- Behavior changed: Yes. Azure Blob Storage data transfers now consume the shared byte-rate limiter when S3 rate limiting is enabled.
- Does this need documentation: No
@0AyanamiRei

Copy link
Copy Markdown
Contributor Author

run buildall

@sollhui

sollhui commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

/review

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.39% (1896/2450)
Line Coverage 64.44% (34064/52862)
Region Coverage 64.87% (17536/27032)
Branch Coverage 54.08% (9404/17390)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review completed for PR #65420.

I did not find a new blocking issue to submit as an inline comment. The earlier Azure byte-accounting gap is already covered by the existing inline thread discussion_r3557395816, and the current head now routes Azure put_object, upload_part, and get_object payload sizes through the shared acquire_s3_rate_limit path.

Critical checkpoint conclusions:

  • Goal/test proof: the PR adds CPU-aware S3 request limits plus byte-rate limiting and centralizes effective limiter calculation. The added BE unit tests cover absolute/per-core/cap/dynamic-reset cases; I could not complete a local BE UT run in this checkout.
  • Scope: the BE changes are focused around S3/Azure object-storage limiter config, helper propagation, and tests.
  • Concurrency/lifecycle: limiter reset continues to use TokenBucketRateLimiterHolder locks; startup use of CpuInfo::num_cores() is after BE CpuInfo::init(), and the BE test harness also initializes CpuInfo.
  • Config/dynamic behavior: new mutable configs have non-negative validators, and check_s3_rate_limiter_config_changed() observes request and byte limiter changes.
  • Compatibility/parallel paths: native S3 and Azure transfer paths now both use the shared request+byte limiter helper. Cloud recycler has its own config surface and was not changed by this BE-scoped PR.
  • Tests/style: git diff --check is clean. I attempted ./run-be-ut.sh --run --filter='S3UTILTest.*', but interrupted it while it was still bootstrapping dependencies/submodules before reaching test execution.
  • User focus: no additional user-provided focus points.

Subagent conclusions:

  • optimizer-rewrite: no optimizer/Nereids surface and no indirect semantic issue found.
  • tests-session-config: no new tests/config/CI candidate found; existing Azure byte-accounting concern is duplicate-covered and fixed in head.
  • Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29332 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 3a87925f6a625fd828c100113a1d320b1f3f8102, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17576	4019	3980	3980
q2	1993	321	203	203
q3	10286	1412	813	813
q4	4722	464	339	339
q5	7739	866	559	559
q6	261	170	135	135
q7	784	839	615	615
q8	10719	1620	1522	1522
q9	6022	4393	4404	4393
q10	6798	1817	1520	1520
q11	506	351	314	314
q12	696	543	421	421
q13	18109	3408	2759	2759
q14	264	264	244	244
q15	q16	781	776	713	713
q17	961	961	945	945
q18	6939	5829	5539	5539
q19	1190	1279	1031	1031
q20	756	664	538	538
q21	5658	2631	2457	2457
q22	414	362	292	292
Total cold run time: 103174 ms
Total hot run time: 29332 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4315	4263	4222	4222
q2	287	310	230	230
q3	4571	4915	4389	4389
q4	2052	2099	1371	1371
q5	4389	4263	4334	4263
q6	221	173	127	127
q7	1730	2118	1667	1667
q8	2396	2165	2056	2056
q9	7856	7747	7755	7747
q10	4719	4696	4260	4260
q11	548	411	380	380
q12	729	832	540	540
q13	3481	3547	3030	3030
q14	315	300	278	278
q15	q16	706	734	654	654
q17	1336	1310	1308	1308
q18	7992	7435	6863	6863
q19	1085	1044	1109	1044
q20	2202	2198	1932	1932
q21	5156	4512	4729	4512
q22	526	463	400	400
Total cold run time: 56612 ms
Total hot run time: 51273 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 180020 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 3a87925f6a625fd828c100113a1d320b1f3f8102, data reload: false

query5	4318	681	496	496
query6	463	229	186	186
query7	4898	592	347	347
query8	334	191	173	173
query9	8767	4106	4108	4106
query10	469	342	322	322
query11	5857	2333	2150	2150
query12	162	115	107	107
query13	1249	623	441	441
query14	6283	5322	4970	4970
query14_1	4314	4306	4325	4306
query15	217	203	184	184
query16	1077	466	444	444
query17	1147	749	595	595
query18	2579	476	360	360
query19	205	189	155	155
query20	110	108	122	108
query21	236	159	134	134
query22	13680	13599	13361	13361
query23	17397	16587	16084	16084
query23_1	16318	16249	16285	16249
query24	7587	1715	1304	1304
query24_1	1321	1289	1284	1284
query25	548	439	367	367
query26	1325	371	205	205
query27	2631	556	370	370
query28	4408	2030	2004	2004
query29	1045	627	481	481
query30	336	253	217	217
query31	1111	1086	982	982
query32	101	60	60	60
query33	507	314	251	251
query34	1162	1202	644	644
query35	768	778	666	666
query36	1404	1405	1268	1268
query37	150	108	89	89
query38	1872	1678	1651	1651
query39	921	926	893	893
query39_1	898	886	890	886
query40	236	158	142	142
query41	67	62	61	61
query42	94	94	95	94
query43	332	323	278	278
query44	1470	778	781	778
query45	195	185	176	176
query46	1039	1219	755	755
query47	2336	2305	2280	2280
query48	414	401	305	305
query49	570	424	313	313
query50	1054	423	328	328
query51	11031	11028	10846	10846
query52	82	90	72	72
query53	256	276	203	203
query54	283	227	248	227
query55	74	73	65	65
query56	300	279	285	279
query57	1423	1425	1316	1316
query58	291	277	246	246
query59	1583	1606	1399	1399
query60	321	279	265	265
query61	172	168	172	168
query62	702	656	578	578
query63	247	208	209	208
query64	2942	1146	963	963
query65	4857	4804	4782	4782
query66	1860	505	390	390
query67	29593	29402	29429	29402
query68	3160	1607	945	945
query69	396	302	275	275
query70	1057	981	1027	981
query71	366	320	313	313
query72	3111	2476	2362	2362
query73	838	749	407	407
query74	5083	4967	4749	4749
query75	2594	2587	2207	2207
query76	2335	1195	816	816
query77	359	390	277	277
query78	12337	12430	11694	11694
query79	1413	1132	802	802
query80	1297	526	468	468
query81	554	314	286	286
query82	602	153	119	119
query83	362	325	292	292
query84	283	163	133	133
query85	945	575	506	506
query86	423	300	290	290
query87	1823	1830	1733	1733
query88	3825	2812	2804	2804
query89	450	401	366	366
query90	1893	210	192	192
query91	204	189	164	164
query92	63	61	60	60
query93	1721	1555	990	990
query94	715	349	321	321
query95	779	498	548	498
query96	1058	794	370	370
query97	2689	2674	2566	2566
query98	208	210	220	210
query99	1134	1181	1063	1063
Total cold run time: 266118 ms
Total hot run time: 180020 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.93 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 3a87925f6a625fd828c100113a1d320b1f3f8102, data reload: false

query1	0.00	0.00	0.00
query2	0.09	0.05	0.06
query3	0.25	0.14	0.14
query4	1.62	0.14	0.14
query5	0.25	0.23	0.22
query6	1.27	1.10	1.06
query7	0.04	0.01	0.01
query8	0.06	0.04	0.04
query9	0.38	0.32	0.32
query10	0.60	0.56	0.53
query11	0.19	0.14	0.14
query12	0.18	0.14	0.14
query13	0.47	0.49	0.47
query14	1.02	1.02	1.03
query15	0.62	0.59	0.59
query16	0.31	0.33	0.34
query17	1.10	1.14	1.09
query18	0.22	0.21	0.21
query19	2.05	1.93	1.86
query20	0.02	0.01	0.01
query21	15.44	0.22	0.13
query22	4.92	0.05	0.05
query23	16.13	0.31	0.12
query24	2.90	0.43	0.31
query25	0.11	0.05	0.05
query26	0.73	0.21	0.15
query27	0.04	0.04	0.03
query28	3.54	0.96	0.60
query29	12.50	4.06	3.22
query30	0.28	0.18	0.16
query31	2.77	0.60	0.31
query32	3.22	0.60	0.48
query33	3.26	3.20	3.29
query34	15.64	4.26	3.49
query35	3.54	3.47	3.55
query36	0.55	0.44	0.42
query37	0.09	0.07	0.06
query38	0.05	0.04	0.04
query39	0.04	0.02	0.03
query40	0.18	0.16	0.16
query41	0.08	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.03	0.04
Total cold run time: 96.83 s
Total hot run time: 24.93 s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants